🦋 Changeset detected

Latest commit: a6d50fd

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

No actionable comments were generated in the recent review.

Merge Risk: Moderate · previous review 416a3

Paid peering settlement now guards against replayed callbacks, and malformed paid terms are rejected. Two retry gaps remain. A completed but unsettled peering invoice can be marked expired after a processor 404 and never retried. A persistently failing oldest batch of invoices can also block retries for later ones. Both affect only paid peering invoices, which no peer can request yet, but they should be fixed before merge.

Security architecture risk: Minimal · up to 416a3

This adds stored billing state but does not yet let peers request invoices or enable payment-gated access. The inspected changes preserve payment isolation and duplicate-settlement protection; no introduced material security exposure was established for this staged release.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The effective shared surface is the relay's invoice store, user balances, and existing payment callbacks. Peering-specific outcomes are bounded to stored peering invoices and their peer ledger rows; the absence of a runtime issuance caller limits exposure in this staged PR.

Trust Boundaries and Controls

• observed — Callback controls remain provider-specific: Nodeless verifies a body HMAC, OpenNode verifies an invoice-ID HMAC, and LNbits verifies its callback token before querying the processor. Zebedee retains its pre-existing optional IP allowlist. These are not equivalent authentication guarantees, and this PR does not strengthen the Zebedee boundary.
• observed — Settlement takes the term from the stored invoice but uses the confirmation object's public key for the peer lock and prefers its unit for amount conversion. Identity and unit consistency therefore remain dependencies on callback and processor provenance rather than explicit stored-owner equality checks.

Resilience and Maintainability Implications

• inferred — Serializable transactions, stored confirmation checks, and the peer-row lock contain concurrent or repeated settlement. Confirmation, entitlement updates, and operator-notification insertion commit together, while the worker supplies recovery for completed peering invoices whose settlement rolled back.

Hardening Proposals

• proposed — Before enabling peer-facing issuance, bind settlement identity and amount units explicitly to the stored invoice and define recovery or reissue behavior for confirmed underpayments.
• proposed — Before populating production peering billing data, establish a downgrade policy that preserves entitlement records and prevents pending peering invoices from becoming ordinary balance-crediting invoices.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "feat(peering): add paid peering billing ledger" matches the main change. The changeset adds the peering_payments ledger, its repository, the billing migration, and peering invoice creation …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "feat(peering): add paid peering billing ledger" matches the main change. The changeset adds the peering_payments ledger, its repository, the billing migration, and peering invoice creation and settlement. The title is specific and follows conventional commit style. It does not describe the peer-facing invoice requests or feed gating, which the PR objectives say are deferred to later PRs, but it does not claim them either.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{A rabbit checks the ledger page,
And marks each peering invoice’s stage.
Terms take shape in satoshis bright,
A paid period extends its flight.
If settlement waits, retries begin,
Then tagged notices hop within.}

Recent review info

Automatic approval policy

Commit: ebf01e61d760.

Setting	Effective value	Source
Automatic approvals	Enabled	Global configuration
Merge risk	Minimal	Global configuration
Security risk	Not set	Global configuration

Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4741bc19-0bab-4b84-9a5e-8525ec936047

Commits

Reviewing files that changed from the base of the PR and between 2d3671c and 416a358.

Files selected for processing: 2

• src/services/payments-service.ts
• test/unit/services/payments-service.spec.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Confidence Score: 5/5

[High impact] The PR appears safe to merge, with no new actionable issues found.

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Create peering invoice] --> B[Store invoice and terms]
  B --> C[Payment callback or worker poll]
  C --> D[Confirm invoice without user credit]
  D --> E{Already confirmed?}
  E -- Yes --> F[Keep existing period]
  E -- No --> G{Paid enough?}
  G -- No --> H[Record underpayment]
  G -- Yes --> I[Start or extend paid period]
  I --> J[Save period on invoice]

_{Reviews (7) · Last reviewed commit: "test(peering): cover the paid peering le..." · Reviewed by Greptile}

Coverage Report for CI Build 43560114628

Coverage increased (+0.2%) to 77.834%

Details

• Coverage increased (+0.2%) from the base build.
• Patch coverage: 9 uncovered changes across 3 files (187 of 196 lines covered, 95.41%).
• No coverage regressions found.

Uncovered Changes

Coverage Regressions

No coverage regressions found.

Coverage Stats

Relevant Lines:	13337
Covered Lines:	10851
Line Coverage:	81.36%
Relevant Branches:	7077
Covered Branches:	5038
Branch Coverage:	71.19%
Branches in Coverage %:	Yes
Coverage Strength:	699.02 hits per line

💛 - Coveralls

PR #1087 — paid peering billing ledger: payment-flow review

Automated review, model litellm/kimi-k3.1-pro, run read-only against the branch head. Every citation below was re-read in the worktree before being reported.
Target: feat/paid-peering-1-ledger at e45cd65, 17 files, +1331/−15 against main.
Scope: the money path only — settlement, idempotency, refunds, units, bounds, tests.

Every citation below was re-read in the worktree before being reported here.

1. Critical — a paid invoice can be permanently stranded between "completed" and "settled"

src/app/maintenance-worker.ts:212-229, src/controllers/callbacks/lnbits-callback-controller.ts:97-100,
src/repositories/invoice-repository.ts:61-63, src/services/payments-service.ts:270-330.

The poll worker writes status = COMPLETED in its own committed call and only then calls
confirmInvoice, which is where the peering settlement happens. The LNbits callback does the
same through updateInvoice, and then refuses every later delivery of that invoice:

if (storedInvoice.status === InvoiceStatus.COMPLETED) {
  response.status(409).send('Invoice is already marked paid')
  return
}

findPendingInvoices selects status = 'pending' only. So if confirmInvoice throws after the
status write — a transient database error, a missing peering_payments row, a failed
updatePeriod — the invoice is completed, unconfirmed and unsettled, the poller never sees it
again, and the callback answers 409 forever. The peer has paid and never receives a period.

The PR's own tests assert this rollback (payments-service.spec.ts:1053, :1070) without noticing
that the rollback leaves the invoice unrecoverable: the transaction is consistent, and the receipt
is dead.

Fix direction: settle before the status leaves pending (or write the status inside the same
transaction), so a failed settlement is retryable rather than terminal.

2. High — a refund does not revoke the access it paid for

src/controllers/callbacks/opennode-callback-controller.ts:79-109, src/services/payments-service.ts:407-451.

refunded maps to InvoiceStatus.EXPIRED and nothing else. The ledger's design makes the peer row
the live access window (status, period_end), and no path reverses them. A peer that pays,
receives a period and is then refunded keeps the period to its end.

3. Medium — an overpayment is forfeited with no record of the surplus

src/services/payments-service.ts:407-451.

amountPaidMsat < termAmountMsats is the only comparison. Paying 42,000 msats against a
21,000-msat term grants exactly one term; the other 21,000 msats are neither credited on the peer
row nor noted anywhere but the invoice's own amount_paid. No test covers it.

4. Low — the invoice-creation boundary repeats none of the term validator's checks

src/services/payments-service.ts:151-198.

resolvePaidPeeringTerm rejects zero, negative, non-integer and sub-satoshi amounts. The public
createPeeringInvoice(peer, 0n, …, 0) does not, so a caller that skips the validator (or
regresses it) persists a zero-value term and settlement then throws at :388-393.

5. Low — a very large term crosses from bigint into an unsafe Number

src/payments-processors/nodeless-payments-processor.ts:39, opennode-payments-processor.ts:37.

Number(amountMsats / 1000n) is exact only below Number.MAX_SAFE_INTEGER sats (~90M BTC). The
term validator accepts any safe integer, so an absurd configured amount issues an invoice for a
rounded value that no longer matches the stored term. Pre-existing code, newly reachable.

6. Medium — the tests cover the easy concurrency and miss the expensive cases

test/unit/services/payments-service.spec.ts:919-1096.

Covered: sequential duplicate callbacks, a repeated callback for an older confirmed invoice,
inner-transaction rollback. Not covered: the worker's earlier committed status, concurrent
callbacks, overpayment, refunds, creation-boundary abuse.

What holds

• Units — configuration requires safe integers and whole satoshis; the domain uses bigint;
  the database round-trips as decimal strings; Math.round affects display text only.
• Double entry — there is no double-entry journal; the invoice is a single-sided receipt and the
  peer row is a stored access window. The two writes are atomic inside one serializable transaction.
• Idempotency — a same-invoice replay is stopped by confirmed_at, serializable isolation and
  SELECT … FOR UPDATE; two distinct paid invoices correctly extend separate periods. Only the
  retry-after-partial-failure path is broken (finding 1).
• Fee split — none exists; the whole term maps to one period, so there is no remainder to place.
• Bounds in the validator — zero means free, and negative, fractional, non-finite and
  sub-satoshi paid terms all fail closed.

Verdict

Fix before merge. Finding 1 is the one to fix: it is the only path that loses a peer's payment
outright, and the fix is ordering rather than redesign. Findings 2 and 3 are policy decisions worth
recording rather than blockers.

Context that lowers the urgency and raises the cost of being wrong: PR #1087 has no consumer yet —
#1088 delivers the invoices and #1089 renews and lapses them — so nothing is live until those land.

PR #1087 — re-review after the settlement fixes

Reviewed by chappie-daemon in the primary session; no model was pinned for this pass. Target: feat/paid-peering-1-ledger at ebf01e6 (was e45cd65 at the first review, which is an ancestor — this is additive). The two author commits are 2d3671c and ebf01e6; the rest of the range is two merges of main and a release. Every citation below was read at ebf01e6.

The first pass's findings, one by one

1. Critical — a paid invoice stranded between completed and settled: fixed.
findUnconfirmedPeeringInvoices selects peering invoices that are COMPLETED with confirmed_at null (src/repositories/invoice-repository.ts:71-92), the worker retries that page ahead of the pending page (src/app/maintenance-worker.ts:199), and needsConfirmation now covers completed-and-unconfirmed as well as a status transition (:252). LNbits answers 409 only when the stored invoice is confirmed (src/controllers/callbacks/lnbits-callback-controller.ts:100), so a redelivery retries the settlement instead of refusing it forever.

The retry is idempotent, which is what makes it safe to run every pass: settlePeeringInvoice returns duplicate when the peer row already names this invoice (src/services/payments-service.ts:437-440), and the row is read under lockByPeer inside the transaction. I also checked that the retry can actually write confirmed_at — it depends on the processor reporting one, and the adapters do on a paid invoice (src/payments-processors/lnbits-payment-processor.ts:78), which confirmInvoice passes to confirm_invoice(?, ?, ?).

Same series, a defect the fix would have exposed: the 404 fallback now expires only a PENDING invoice (src/app/maintenance-worker.ts:283), so a completed peering settlement cannot be marked expired underneath the retry.

2. High — a refund does not revoke the period: recorded as policy. The mapping carries the decision and its reason (src/controllers/callbacks/opennode-callback-controller.ts:81-84). That was the ask; no objection.

3. Medium — an overpayment is forfeited: addressed as far as the design allows. The receipt keeps the processor's amount instead of the requested one (src/app/maintenance-worker.ts:262-271), the peer row is still not credited, and the choice is written down (src/services/payments-service.ts:467-469). Covered by "grants exactly one term for an overpayment and keeps the full amount on the receipt" (test/unit/services/payments-service.spec.ts:1049).

4. Low — the creation boundary repeated none of the validator's checks: fixed (src/services/payments-service.ts:176-192): positive whole satoshis, a safe-integer period, and the range check.

5. Low — a large term crossing into an unsafe Number: fixed in both places, the validator (src/utils/peering-billing.ts:93) and the boundary (src/services/payments-service.ts:187).

6. Medium — the tests covered the easy cases: largely addressed. New tests cover the retry, the cursor advance and reset, the 404 regression, the settlement-time period start, overpayment and creation-boundary abuse, each in the spec that owns it. Still uncovered: two confirmations of one invoice running concurrently. The argument for it — serializable transaction, lockByPeer, lastSettledInvoiceId — is sound, but it remains a reading, not a result.

New, none blocking

N1 — a settlement that keeps throwing is now retried forever, silently. The worker catches, logs and moves on (src/app/maintenance-worker.ts:303); nothing is enqueued and no attempt is counted. settlePeeringInvoice throws on a missing peer row or a missing term snapshot (src/services/payments-service.ts:426-434), and only the underpaid branch writes last_error. Before this PR such an invoice was stranded once; now it is retried every pass with no operator-visible signal. A peering.payment.failed notification after N attempts, or a counter on the row, would make the failure visible.

N2 — the retry set is gated on COMPLETED, and the pass writes the processor's status before deciding to confirm. updateInvoiceStatus at src/app/maintenance-worker.ts:250 runs before needsConfirmation at :252, so an invoice whose settlement rolled back leaves the retry set the moment the processor reports anything other than completed — an expired LNbits invoice, for instance. The guarantee therefore holds only while the processor keeps reporting paid. Deciding the confirmation before the downgrade, or refusing to downgrade a completed, unconfirmed peering invoice, would close the gap.

N3 — a note on the cursor, not a defect. It advances a full page when the page is full (src/app/maintenance-worker.ts:218), while successful retries shrink the set, so an invoice that shifts into the passed window waits until a short page resets the cursor to 0. A delay, never a loss; worth one line in the comment so a reader does not take the cursor for exact.

Check state at ebf01e6

Build, Lint, Unit Tests, Integration Tests, Changeset and Greptile all pass. Coveralls - Integration is red: coverage decreased 0.4% to 32.424%.

Verdict

The blocker from the first review is cleared, and both policy decisions are recorded in the code rather than in a comment on this PR. N1 and N2 are worth an issue, not another round. I am not approving in this pass because a check is red (the integration coverage one, which is about coverage rather than this diff) — the merge decision stays with the repository owner.